-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[expressions] caching #180440
[expressions] caching #180440
Conversation
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
What a lovely PR improving the experience of our apps 😍 I was mostly focused on the ES|QL experience which was works exactly as I am expecting. I also tested the dataview mode and everything seems to work great except from annotations. Here I tried to add an annotations layer from the dashboard inline editing. @dej611 @mbondyra I think it would be great both of you to test this PR to ensure that everything works as expected! |
@ppisljar I can't reproduce it now 🤔 Annotations work great in my new tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one bug only is very good news 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing an odd behavior:
- Add a delay to the query
- Let the query complete, then change the colors (they change just fine without re-issuing the query)
- Refresh the query
- Before the query completes, try to change the colors (you get "no results found")
I tried to reproduce on main and I couldn't, so I'm pretty sure this has to do with this PR. It looks like the query is being cancelled for some reason.
Untitled.mov
good catch @lukasolson ! I updated the code with a check to make sure we only cache the last result from the observable and we are not caching partial results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested out again and cannot find issues so far 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes looks good to me, great work here!
Summary
adds caching support to expressions. Functions can set
allowCaching
property totrue
to indicate they can/should be cached.When an expression is executed with
allowCaching
parameter set totrue
the expression engine will cache results of such functions and reuse them in later executions of expression if:fix #177548
Checklist
Delete any items that are not applicable to this PR.